Skip to content

Sort order typo #714

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Sort order typo #714

wants to merge 2 commits into from

Conversation

lukapor
Copy link
Contributor

@lukapor lukapor commented Jun 13, 2014

No description provided.

@Mpdreamz Mpdreamz added this to the NEST 1.0 RC milestone Jun 16, 2014
@gmarz
Copy link
Contributor

gmarz commented Jul 8, 2014

Hey @lukapor, thank you for this. Your PR inspired #748, which defines a SimilarityDescriptor and provides the ability to set similarities via the fluent API approach that all our descriptors take.

Regarding your change to SortFieldDescriptor, I believe ToggleSort is correct the way it is. It's toggling the sort order, not just setting it- so if the current order is Ascending, then it would be correct to change it to Descending.

Thanks again for your PR, sorry I couldn't merge it in!

@gmarz gmarz closed this Jul 8, 2014
@lukapor
Copy link
Contributor Author

lukapor commented Jul 8, 2014

Hey ok it is maybe confusing method name, but it isn't consistent with same function in SortScriptDescriptor.cs and SortFieldDescriptor.cs. Please check it.

@lukapor
Copy link
Contributor Author

lukapor commented Jul 8, 2014

And SortGeoDistanceDescriptor.cs file.

@gmarz
Copy link
Contributor

gmarz commented Jul 9, 2014

@lukapor You are totally right, thanks for pointing that out. ToggleSort is completely confusing. I just opened PR #764 which replaces ToggleSort() with Order() and takes the SortOrder enum instead of a bool. I think that's much clearer and just as effective- what do you think?

@lukapor
Copy link
Contributor Author

lukapor commented Jul 9, 2014

That is great. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants